feat: add account_advisory cleanup to advisory cleanup job - #2290
feat: add account_advisory cleanup to advisory cleanup job#2290katarinazaprazna wants to merge 2 commits into
Conversation
Reviewer's GuideAdds a new parallel cleanup job for legacy advisory_account_data and the new account_advisory table, updates the unused-advisories deletion query to respect the new table, wires a new job entrypoint/cron config, and adds a DB test ensuring metadata referenced by account_advisory is preserved. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2290 +/- ##
==========================================
- Coverage 59.12% 58.97% -0.16%
==========================================
Files 149 149
Lines 9549 9575 +26
==========================================
+ Hits 5646 5647 +1
- Misses 3311 3336 +25
Partials 592 592
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d447030 to
5c64f33
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- RunCleanAccountAdvisory always logs
task performed successfullyeven if one or both cleaners fail; consider aggregating error results and surfacing a non-success log or exit status when any delete operation returns an error. - CleanAdvisoryAccountData and CleanAccountAdvisory share nearly identical transaction and logging logic; consider extracting a small helper that accepts the model and condition to reduce duplication and keep the behavior consistent across both tables.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- RunCleanAccountAdvisory always logs `task performed successfully` even if one or both cleaners fail; consider aggregating error results and surfacing a non-success log or exit status when any delete operation returns an error.
- CleanAdvisoryAccountData and CleanAccountAdvisory share nearly identical transaction and logging logic; consider extracting a small helper that accepts the model and condition to reduce duplication and keep the behavior consistent across both tables.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
5c64f33 to
3a644c6
Compare
3a644c6 to
092f42d
Compare
Extend the advisory cleanup job to also clean zero-count rows from the new account_advisory table, running both cleanups in parallel. The legacy advisory_account_data cleanup runs until that table is dropped.
092f42d to
27e51dd
Compare
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
CleanAdvisoryAccountDataandCleanAccountAdvisoryfunctions are nearly identical; consider extracting a shared helper that takes the model type/table name to reduce duplication and keep the cleanup logic consistent in one place. - In both cleanup functions you call
Begin()withdefer tx.Rollback()and thentx.Commit()without checking its error; consider either using the base DB without an explicit transaction for this single-statement delete, or handlingCommit()errors explicitly and avoiding a deferred rollback after a successful commit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `CleanAdvisoryAccountData` and `CleanAccountAdvisory` functions are nearly identical; consider extracting a shared helper that takes the model type/table name to reduce duplication and keep the cleanup logic consistent in one place.
- In both cleanup functions you call `Begin()` with `defer tx.Rollback()` and then `tx.Commit()` without checking its error; consider either using the base DB without an explicit transaction for this single-statement delete, or handling `Commit()` errors explicitly and avoiding a deferred rollback after a successful commit.
## Individual Comments
### Comment 1
<location path="tasks/cleaning/clean_account_advisory.go" line_range="11-20" />
<code_context>
+ "sync"
+)
+
+func RunCleanAccountAdvisory() {
+ tasks.HandleContextCancel(tasks.WaitAndExit)
+ core.ConfigureApp()
+ defer utils.LogPanics(true)
+
+ var wg sync.WaitGroup
+ wg.Add(2)
+
+ go func() {
+ defer wg.Done()
+ utils.LogInfo("Deleting advisory rows with 0 applicable/installable systems from advisory_account_data")
+ if err := CleanAdvisoryAccountData(); err != nil {
+ utils.LogError("err", err, "Cleaning advisory_account_data")
+ }
+ }()
+
+ go func() {
+ defer wg.Done()
+ utils.LogInfo("Deleting advisory rows with 0 applicable/installable systems from account_advisory")
+ if err := CleanAccountAdvisory(); err != nil {
+ utils.LogError("err", err, "Cleaning account_advisory")
+ }
+ }()
+
+ wg.Wait()
+ utils.LogInfo("RunCleanAccountAdvisory task performed successfully")
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider surfacing failures from the two cleanup routines instead of always logging success
Right now, errors from `CleanAdvisoryAccountData` / `CleanAccountAdvisory` are only logged inside the goroutines, but `RunCleanAccountAdvisory` still logs a blanket success message. This can misrepresent job status and interfere with alerting. Consider aggregating errors (e.g., via a channel or shared error with synchronization) so that you can emit a failure log when any cleanup fails and optionally exit with a non-zero status for the job runner.
</issue_to_address>
### Comment 2
<location path="tasks/cleaning/clean_account_advisory.go" line_range="39-48" />
<code_context>
+ utils.LogInfo("RunCleanAccountAdvisory task performed successfully")
+}
+
+func CleanAdvisoryAccountData() error {
+ tx := tasks.CancelableDB().Begin()
+ defer tx.Rollback()
+
+ result := tx.Delete(&models.AdvisoryAccountData{}, "systems_installable <= 0 AND systems_applicable <= 0")
+ if result.Error != nil {
+ return result.Error
+ }
+
+ tx.Commit()
+ utils.LogInfo("nDeleted", result.RowsAffected, "advisory_account_data cleaned successfully")
+ return nil
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid deferring Rollback after a successful Commit and check Commit errors
With `Begin` + `defer tx.Rollback()` + `tx.Commit()`, `Rollback` still runs after `Commit`, which can generate spurious errors/logs depending on the driver, and `Commit` errors are currently ignored.
Prefer an explicit pattern that:
- Checks `Begin` error
- Uses `defer` only for panic recovery
- Rolls back on intermediate errors
- Checks and returns the `Commit` error
For example:
```go
func CleanAdvisoryAccountData() error {
tx := tasks.CancelableDB().Begin()
if tx.Error != nil {
return tx.Error
}
defer func() {
if r := recover(); r != nil {
tx.Rollback()
panic(r)
}
}()
result := tx.Delete(&models.AdvisoryAccountData{}, "systems_installable <= 0 AND systems_applicable <= 0")
if result.Error != nil {
tx.Rollback()
return result.Error
}
if err := tx.Commit().Error; err != nil {
return err
}
utils.LogInfo("nDeleted", result.RowsAffected, "advisory_account_data cleaned successfully")
return nil
}
```
Same fix applies to `CleanAccountAdvisory()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| func CleanAdvisoryAccountData() error { | ||
| tx := tasks.CancelableDB().Begin() | ||
| defer tx.Rollback() | ||
|
|
||
| result := tx.Delete(&models.AdvisoryAccountData{}, "systems_installable <= 0 AND systems_applicable <= 0") | ||
| if result.Error != nil { | ||
| return result.Error | ||
| } | ||
|
|
||
| tx.Commit() |
There was a problem hiding this comment.
issue (bug_risk): Avoid deferring Rollback after a successful Commit and check Commit errors
With Begin + defer tx.Rollback() + tx.Commit(), Rollback still runs after Commit, which can generate spurious errors/logs depending on the driver, and Commit errors are currently ignored.
Prefer an explicit pattern that:
- Checks
Beginerror - Uses
deferonly for panic recovery - Rolls back on intermediate errors
- Checks and returns the
Commiterror
For example:
func CleanAdvisoryAccountData() error {
tx := tasks.CancelableDB().Begin()
if tx.Error != nil {
return tx.Error
}
defer func() {
if r := recover(); r != nil {
tx.Rollback()
panic(r)
}
}()
result := tx.Delete(&models.AdvisoryAccountData{}, "systems_installable <= 0 AND systems_applicable <= 0")
if result.Error != nil {
tx.Rollback()
return result.Error
}
if err := tx.Commit().Error; err != nil {
return err
}
utils.LogInfo("nDeleted", result.RowsAffected, "advisory_account_data cleaned successfully")
return nil
}Same fix applies to CleanAccountAdvisory().
27e51dd to
82efcf3
Compare
Summary
Extend the advisory cleanup job to also clean zero-count rows from the new
account_advisorytable, running both cleanups in parallel until the legacy table is droppedAdd row-count logging to cleanups for observability
Prevent deleting
advisory_metadatathat still has rows in the new table. However,account_advisoryfans out across 32 hash partitions per candidate, bounded byLIMIT 1000. Do you think we should validate this on stage before merging, or are we good?Follow-up
PUT /clean-advisory-account-dataadmin API endpoint only cleans the legacy table. Update to also cleanaccount_advisory(or rename/replace when legacy table is dropped)Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Add a new job that concurrently cleans legacy and new account advisory data while ensuring advisory metadata is only removed when unused by both tables.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: